-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DOMAIN_NAME for networks #34
base: master
Are you sure you want to change the base?
Conversation
@@ -126,6 +126,8 @@ private function getHostsLines($container) | |||
// Networks | |||
if (isset($container['NetworkSettings']['Networks']) && is_array($container['NetworkSettings']['Networks'])) { | |||
foreach ($container['NetworkSettings']['Networks'] as $networkName => $conf) { | |||
preg_match(sprintf('/^%s_(.+)$/', $this->getProjectName($container)), $networkName, $matches); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it working with a container not created by docker compose ?
docker run ...
@iamluc What the state of this PR? I would love to see it merged :) |
ping @Jean-Beru |
Not really but I will do an effort just fo you @iamluc ;-) |
Hi there, |
./bin/docker-hostmanager PHP Fatal error: Uncaught Error: Call to undefined method DockerHostManager\Synchronizer::getAdditionalHosts() in src/Synchronizer.php:181
Update from mykiwi
Note: it seems that using this patch does work, but you need to specify the network, like so: web:
environment:
- DOMAIN_NAME=default:foo.bar Without the |
@iamluc what can we do to get this built and pushed to Docker Hub? :) |
Hi @dkarlovi thanks for testing. I'm gonna try to finish this PR with @Jean-Beru and @mykiwi in a few days. |
@@ -126,6 +126,8 @@ private function getHostsLines($container) | |||
// Networks | |||
if (isset($container['NetworkSettings']['Networks']) && is_array($container['NetworkSettings']['Networks'])) { | |||
foreach ($container['NetworkSettings']['Networks'] as $networkName => $conf) { | |||
preg_match(sprintf('/^%s_(.+)$/', $this->getProjectName($container)), $networkName, $matches); | |||
$networkShortName = isset($matches[1]) ? $matches[1] : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$networkShortName = isset($matches[1]) ? $matches[1] : 'default';
should allow to not require setting the name as DOMAIN_NAME=default:foo.bar
if the default network is used.
BTW I've pushed this to https://hub.docker.com/r/dkarlovi/docker-hostmanager/ as a workaround, will remove it once this gets merged. |
@iamluc any news on this? |
I discussed with @Jean-Beru about this PR, and the need is not clearly defined (IMHO). Could you tell if those examples help you to do what you want to achieve ? Any help to improve the documentation is also welcomed 😄 |
@iamluc hostmanager adds some non-human-friendly Docker hosts to
The goal here is to allow define user-friendly one(s) so it can be accessed and configured as developer wants, so instead of the lines above we can get:
and use |
@Wirone
Will have the same result. Of course you need to create the |
This makes the process a bit more complex in a cross-cutting team, as you must always ship additional tooling too. It might also lead to collisions (where multiple projects are trying to create the same external network). Also, it doesn't allow you to have different / multiple / no suffixes if the need arises, if I understood correctly. |
In fact, this is not needed anymore. Since compose file version 3.5, you can change the name of the network (https://docs.docker.com/compose/compose-file/#name-1).
You can have define multiple networks, it will result to multiple suffix.
Indeed a suffix is required, but I don't think it is a good idea to not have one. A very important point to me is that with this system, hostnames are the same whether you are inside a container or outside from you host, because we do the same thing docker does. If we allow changing the hostname with a label or an env var, it will works ONLY from the host |
That does sound neat. But, would it lead to problems? For example, I wish to always use the suffix Also, note that I'm using a actual valid FQDN for some of my projects to allow them to have valid Let's Encrypt certs (example, |
Hi @iamluc, have you decided how to proceed here? |
IMHO, this feature is not needed so I will not work on it. BUT, if someone wants to finish it, validates it is working and ensures it breaks nothing. I will agree to merge it. |
We've been using this since I've pushed my own image with the PR merged. It does work as expected. |
Yep, but it would be great if someone could reply to the comments: And also document this feature in the README |
Fair 👍 I try to get this done in the near future. |
also, please change the target of the PR to the |
@@ -136,6 +138,12 @@ private function getHostsLines($container) | |||
$hosts[] = $alias.'.'.$networkName; | |||
} | |||
|
|||
foreach ($this->getAdditionalContainerHosts($container) as $host) { | |||
if (preg_match('/^(.+):(.+)$/', $host, $matches) && $matches[1] === $networkShortName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this check and not directly add the host in the array ?
{ | ||
$hosts = []; | ||
|
||
$domainName = $this->getEnv($container, 'DOMAIN_NAME'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to suggest that DOMAIN_NAME
is not entirely correct name for what we want to achieve with this feature. "Domain" is rather HTTP concept, but host manager can be used also for database containers instead of port binding (so developer can connect to database with client by custom host, not by localhost
with bound port).
@dkarlovi @iamluc would be good to finish this somehow... is there something we can do to get this PR merged? Using Dalibor's fork is OK, but maintaining it (keeping up-to-date) isn't something easy I think. Additionally, Dalibor's fork does NOT contain changes in readme, so sharing link to the repo isn't enough (I just sent it to my colleague and he installed |
Gotta admit I've forgotten about my promise here. I'm using my image ever since I've pushed it, with my entire team (we've documented it in our project) so it's basically fire & forget currently. @Wirone If you feel like you'd get additional value from this being merged, maybe you can take over the PR and get it merged? |
@dkarlovi how is it possible technically? PR is from @Jean-Beru's branch, if we want to continue in this PR I would need to get access to that repo 🤔? Or maybe you mean forking fork and creating new PR? |
You just fork this repo, add @Jean-Beru's repo as an additional remote and checkout the PR branch into your own. Any commit you make would of course be made to your own branch which gets a separate PR, but all the code is already there. Otherwise you'd need to do PRs against that repo which is inefficient. There's some Github magic with refs which allows you to do it automagically, but I always forget how. |
Yo, do we have to drop this one in favor of dkarlovi's one if this feature does not workas expected ? 😮 |
Resolves #32
Add the possibility to add a DOMAIN_NAME for containers in a network. To specify the network concerned by the domain name, add
my_network_name:
before value. Exemple :will result in